Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Submodules with simpler APIs to Pp_ast #537

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

NathanReb
Copy link
Collaborator

This adds a functor and a Default module, similar to what's done in Ast_builder. One can use the functor to get pre-configured formatters or use the formatters provided by the Default module which are preconfigured with the default configuration.

This provides modules whose pretty printers can be used directly with "%a" format strings as they don't expect an optional argument first.

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me.

Is there any benefit to doing this the Ast_builder way fully by extracting the type interface and using a 'a with_loc and 'a without_location type (except for with and without a config) ?

@NathanReb
Copy link
Collaborator Author

Yes that sounds like a good idea! I'll factor out the pretty-printer type!

I'll go ahead and make the test suite 5.3 compatible first though!

@NathanReb
Copy link
Collaborator Author

I pushed the changes with the new alias types. I didn't go for with/without_config as I found those names a bit ambiguous. One could read with_config either as with a config argument or with an embedded configuration. I used configured and configurable instead as I found those would leave less room for interpretation.

@mbarbin
Copy link
Contributor

mbarbin commented Nov 26, 2024

@NathanReb did you mean to reply to #338 ? (your comment makes more sense to me in the context of this other issue)

@NathanReb
Copy link
Collaborator Author

@NathanReb did you mean to reply to #338 ? (your comment makes more sense to me in the context of this other issue)

Indeed! My bad, I'll move the message!

@NathanReb NathanReb merged commit 0d9893b into ocaml-ppx:main Nov 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants